Skip to content

Recommend declare instead of disabling strictPropertyInitialization#145

Merged
keithamus merged 3 commits intogithub:mainfrom
chriskrycho:patch-1
Oct 19, 2021
Merged

Recommend declare instead of disabling strictPropertyInitialization#145
keithamus merged 3 commits intogithub:mainfrom
chriskrycho:patch-1

Conversation

@chriskrycho
Copy link
Copy Markdown
Contributor

@chriskrycho chriskrycho commented May 6, 2021

Since TypeScript 3.7, which was released in November 2019, TypeScript supports declaring properties which will be initialized in a way that is invisible to TS via the declare property modifier. This enables using strictPropertyInitialization with class or field decorators (as well as other patterns which can initialize classes invisibly to TS).

Accordingly, this PR suggests switching to declare and makes that the first recommended path.

If you all prefer to keep the recommendation to switch off strictPropertyInitialization (which means other properties can be accidentally uninitialized!), I'd strongly recommend using declare in your workaround guidance instead of !, because ! is not forward-compatible with the ES spec (see here for details).

Since TypeScript 3.7, which was released in November 2019, TypeScript
supports declaring properties which will be initialized in a way that
is invisible to TS via the `declare` property modifier. This enables
using `strictPropertyInitialization` with class or field decorators (as
well as other patterns which can initialize classes invisibly to TS).
@chriskrycho chriskrycho requested a review from a team as a code owner May 6, 2021 23:03
@koddsson
Copy link
Copy Markdown
Contributor

koddsson commented May 7, 2021

I'd strongly recommend using declare in your workaround guidance instead of !, because ! is not forward-compatible with the ES spec (see here for details).

Is declare forward compatible with the ES spec? I could not find a reference to it in the ECMA262.

@chriskrycho
Copy link
Copy Markdown
Contributor Author

chriskrycho commented May 7, 2021

Yeah. The reason is a bit subtle; the forward compatibility here is not "will this thing be allowed in this position?" (to which the answer is "who knows but probably no" for both of them!), but rather "what are the semantics of the field created this way?"

For !, it creates and initialized the property as undefined using defineProperty in the constructor, which stomps any storage that was already there (via the prototype, a superclass etc.) and is very sensitive to the order of the operations which modify the class field as a result. That is "correct" in that it's how a class field with no initializer is specced but it's basically never what you want when working with a decorator.

declare simply no-ops the actual field creation from TS's side, until the decorator does its job and creates it with its own details; there's never a defineProperty call which initializes the field as undefined in that flow. That means that you can safely use declare for any instance of this kind of thing and you'll always get the right behavior. Also, that's true whether you emit your compiled JS using TS itself or using Babel!

Copy link
Copy Markdown
Contributor

@keithamus keithamus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @chriskrycho thanks for this change! This is really well worded and fits within our documentation style. Kudos!

Sorry this took so long to merge. We're trying to find more ergonomic ways to handle this, but I think ultimately, for now, this accurately describes the solutions and so we should definitely make it visible.

@chriskrycho
Copy link
Copy Markdown
Contributor Author

All good – thanks for getting back around to it. (I have more than a few long-standing PRs to get back to on things I maintain myself. 😂)

Comment thread docs/_guide/decorators.md Outdated
@keithamus keithamus merged commit 7a44cff into github:main Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants